Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean dependencies #239

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Clean dependencies #239

wants to merge 4 commits into from

Conversation

moznuy
Copy link
Contributor

@moznuy moznuy commented May 6, 2022 •
edited
Loading

Fixes #234

Changes

  • Clean dependencies:
    • Make hiredis, email-validator optional for public and required for dev dependencies.
    • Add hiredis, email-validator to package extras.
    • Remove pptree, cleo, six from public dependencies.
    • Remove ipdb, pylint from dev dependencies.
    • Move types-redis, types-six to dev dependencies.
    • Change RedisModel.from_redis to Python 3 syntax for six removal.
  • Add documentation about optional dependencies.
  • Run make lint format test.
  • Run poetry update?
  • Sort dependencies?
  • Remove types-six? (six is removed)
  • Is pytest-xdist needed?
  • Is tox(with tox-env) needed?

Comparison of dist/<>/setup.py:

Before

install_requires = [
    "aioredis>=2.0.0,<3.0.0",
    "cleo==1.0.0a4",
    "click>=8.0.1,<9.0.0",
    "hiredis>=2.0.0,<3.0.0",
    "pptree>=3.1,<4.0",
    "pydantic>=1.8.2,<2.0.0",
    "python-ulid>=1.0.3,<2.0.0",
    "redis>=3.5.3,<5.0.0",
    "six>=1.16.0,<2.0.0",
    "types-redis>=3.5.9,<5.0.0",
    "types-six>=1.16.1,<2.0.0",
    "typing-extensions>=4.0.0,<5.0.0",
]

After

install_requires = [
    "aioredis>=2.0.0,<3.0.0",
    "click>=8.0.1,<9.0.0",
    "pydantic>=1.8.2,<2.0.0",
    "python-ulid>=1.0.3,<2.0.0",
    "redis>=3.5.3,<5.0.0",
    "typing-extensions>=4.0.0,<5.0.0",
]

extras_require = {
    "email": ["email-validator>=1.2.1,<2.0.0"],
    "hiredis": ["hiredis>=2.0.0,<3.0.0"],
}

@moznuy
Copy link
Contributor Author

moznuy commented May 12, 2022

Any input will be appreciated

@moznuy moznuy mentioned this pull request Jun 13, 2022
Open
@ikornaselur
Copy link

Are there any updates on this? #233 has been merged, which means that aioredis can be removed here as well.

As for the outstanding todo tasks on this PR it seems that they mostly are referring to dev dependencies (questions related to tox, pytest and even sorting dependencies).

I'd love to see at least the removal of aioredis released soon as it's not a dependency that is used anymore and I'd hate to see it installed when using this package when it's not used anywhere.

Not sure if I could contribute in any way to help get this in, but as for the questions I could provide some info for:

Remove types-six? (six is removed)

I believe this makes sense to remove, seeing as six isn't being used in the package anymore.

Is pytest-xdist needed?

This package provides concurrency to tests being run, which is used in both test and test_oss make commands. pytest -n auto tells pytest ot run with 'auto' number of workers (count depends on system running the tests).

Is tox(with tox-env) needed?

It doesn't look like tox is being used? CI runs on Github Actions with a matrix over different Python versions, though it might still be useful to run manually before it hits CI. It's not documented anywhere though.

Poetry 1.2+ (IIRC) introduces dependency groups instead of just main + dev, which might make sense to utilise for optional local development stuff like this. Or at least have some way of just optionally installing these local development tools. I guess dev dependencies can be marked as optional, just as main dependencies can be?

@moznuy
Copy link
Contributor Author

moznuy commented Nov 21, 2022

Are there any updates on this? #233 has been merged, which means that aioredis can be removed here as well.

The problem was/is that the communication between maintainers and contributors was non existent (maybe something has changed). I would glad to come back and rebase onto HEAD / finish PR, if there is a chance this might be merged.

This package provides concurrency to tests being run, which is used in both test and test_oss make commands. pytest -n auto tells pytest ot run with 'auto' number of workers (count depends on system running the tests).

As far as I remember, it was faster for me to run tests without concurrency than with it. The question is "is it really needed?", as stated in the #234.

@ikornaselur
Copy link

As far as I remember, it was faster for me to run tests without concurrency than with it. The question is "is it really needed?", as stated in the #234.

Fair enough! Hadn't noticed that point in #234 - the test suite seems to be fairly quick already, just a minute for the suite if looking at the CI run, so personally I don't really see the use of xdist.. especially if it's slower in practice! 😄

The problem was/is that the communication between maintainers and contributors was non existent (maybe something has changed). I would glad to come back and rebase onto HEAD / finish PR, if there is a chance this might be merged.

I was wondering about this yesterday, if there was any public place for discussion outside of just GitHub issues, but couldn't find any at a cursory glance. It's definitely frustrating to have things in limbo, so hopefully communication will improve 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clean dependencies
2 participants
@moznuy @ikornaselur